-
Notifications
You must be signed in to change notification settings - Fork 477
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Report encode/decode time and implementation metric #2434
Conversation
2103581
to
c731c87
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm just some naming nits
@@ -4,4 +4,5 @@ | |||
export default class GlobalMetricReport { | |||
previousMetrics: { [id: string]: number } = {}; | |||
currentMetrics: { [id: string]: number } = {}; | |||
stringValues: { [id: string]: string } = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe currentDimensions
or just dimensions
instead of stringValues
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think they are just dimension, it can also be metric derived from string value. For example, whether decoder/encoder is in hardware is one of the derived metric.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it to currentStringMetrics
. Also added comments that this is a workaround in case metric values are strings.
@@ -10,4 +10,5 @@ export default class StreamMetricReport { | |||
direction: Direction; | |||
previousMetrics: { [id: string]: number } = {}; | |||
currentMetrics: { [id: string]: number } = {}; | |||
stringValues: { [id: string]: string } = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it to currentStringMetrics
. Also added comments that this is a workaround in case metric values are strings.
@@ -297,6 +333,14 @@ export default class ClientMetricReport { | |||
}, | |||
frameHeight: { transform: this.identityValue, type: SdkMetric.Type.VIDEO_DECODE_HEIGHT }, | |||
frameWidth: { transform: this.identityValue, type: SdkMetric.Type.VIDEO_DECODE_WIDTH }, | |||
totalDecodeTime: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this name be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This key is just about the field name in webrtc report where the metric is derived from, we cannot change that naming as it comes from webrtc..
/** | ||
* Dimensions derived from metric | ||
*/ | ||
readonly streamMetricDimensionMap: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Don't think we need 'Metric' in any of this or related naming?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated naming as suggested.
add implementation name as a dimension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -126,6 +126,35 @@ export default class ClientMetricReport { | |||
return Number(metricReport.currentMetrics[metricName] * 1000); | |||
}; | |||
|
|||
millisecondsPerSecond = (metricName?: string, ssrc?: number): number => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we name this to be clearer what it does? milliseconds per second is kinda confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comment. Created #2443 to update function naming
Report encode/decode time and if encoder/decoder is in hardware, and add encoder/decoder name as a metric dimension Co-authored-by: Shi Su <[email protected]>
add implementation name as a dimension.
Issue #:
Description of changes:
Report encode/decode time and if encoder/decoder is in hardware, and add encoder/decoder name as a metric dimension
Testing:
Added unit tests to cover the changes
Can these tested using a demo application? Please provide reproducible step-by-step instructions.
Join a meeting with Chrome and select log level as
DEBUG
, turn on video on one user. Use the following string to filter console logs[DEBUG] SDK - sending:
. Verify the log contains the following fields on sender and receiver side, respectively.Sender side:
VIDEO_ENCODE_MS
metric field. Example:VIDEO_ENCODER_NAME
dimension. Example:Message example:
Receiver side:
VIDEO_DECODE_MS
metric field. Example:VIDEO_DECODER_NAME
dimension. Example:Message example:
Checklist:
Have you successfully run
npm run build:release
locally?y
Do you add, modify, or delete public API definitions? If yes, has that been reviewed and approved?
n
Do you change the wire protocol, e.g. the request method? If yes, has that been reviewed and approved?
n
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.